Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jobspec: add a chown option to artifact block #24157

Merged
merged 2 commits into from
Oct 11, 2024
Merged

jobspec: add a chown option to artifact block #24157

merged 2 commits into from
Oct 11, 2024

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Oct 9, 2024

This PR adds a boolean chown field to the artifact block.

It indicates whether the Nomad client should chown the downloaded files
and directories to be owned by the task.user. This is useful for drivers
like raw_exec and exec2 which are subject to the host filesystem user
permissions structure. Before, these drivers might not be able to use or
modify the downloaded artifacts since they would be owned by the root
user on a typical Nomad client configuration.

artifact {
  source      = "https://example.org/blah/Hello.java"
  destination = "local/java"
  chown       = true
}

In this example the local/java directory will recursively become owned by the task's user instead of root.

Fixes hashicorp/nomad-driver-exec2#51

@shoenig
Copy link
Member Author

shoenig commented Oct 10, 2024

not sure what's up with test-e2e-vault but i'm sure it's not related

failed to download vault 1.17.6: hc-install: will install vault@1.17.6
failed to install vault@1.17.6: unexpected Content-Type: "application/vnd+hashicorp.releases-api.v1+json"

@shoenig shoenig marked this pull request as ready for review October 10, 2024 17:21
@shoenig shoenig marked this pull request as draft October 10, 2024 17:22
This PR adds a boolean 'chown' field to the artifact block.

It indicates whether the Nomad client should chown the downloaded files
and directories to be owned by the task.user. This is useful for drivers
like raw_exec and exec2 which are subject to the host filesystem user
permissions structure. Before, these drivers might not be able to use or
manage the downloaded artifacts since they would be owned by the root
user on a typical Nomad client configuration.
@tgross
Copy link
Member

tgross commented Oct 10, 2024

not sure what's up with test-e2e-vault but i'm sure it's not related

I've been seeing it fail intermittently today with that hc-install issue. Must be something in the artifact server.

@@ -1426,6 +1426,7 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup,
GetterMode: *ta.GetterMode,
GetterInsecure: *ta.GetterInsecure,
RelativeDest: *ta.RelativeDest,
Chown: *ta.Chown,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This panics if ta.Chown is nil, which will be the case if we receive a HTTP request outside of the Go API because the zero value for the unset field is now nil instead of false. (But see also my note on the API struct.)

api/tasks.go Outdated
Comment on lines 887 to 889
if a.Chown == nil {
a.Chown = pointerOf(false)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to canonicalize it to the zero-value (false) anyways, we can make it bool with the tag hcl:"chown,optional" instead of a *bool and avoid downstream nil checking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Fixed.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shoenig shoenig merged commit f1ce127 into main Oct 11, 2024
27 of 28 checks passed
@shoenig shoenig deleted the artifact-chown branch October 11, 2024 16:30
Juanadelacuesta pushed a commit that referenced this pull request Nov 4, 2024
* jobspec: add a chown option to artifact block

This PR adds a boolean 'chown' field to the artifact block.

It indicates whether the Nomad client should chown the downloaded files
and directories to be owned by the task.user. This is useful for drivers
like raw_exec and exec2 which are subject to the host filesystem user
permissions structure. Before, these drivers might not be able to use or
manage the downloaded artifacts since they would be owned by the root
user on a typical Nomad client configuration.

* api: no need for pointer of chown field
@ahjohannessen
Copy link

ahjohannessen commented Nov 14, 2024

@tgross @shoenig

I got this on Flatcar Linux this morning:

failed to download artifact "https://github.com/grpc-ecosystem/grpc-health-probe/releases/download/v0.4.35/grpc_health_probe-linux-amd64": getter subprocess failed: exit status 1: failed to download artifact: Get "https://github.com/grpc-ecosystem/grpc-health-probe/releases/download/v0.4.35/grpc_health_probe-linux-amd64": tls: failed to verify certificate: x509: failed to load system roots and no roots provided; open /etc/ssl/certs/ca-certificates.crt: permission denied

Seems something changed with regards to artifact permissions to read certificates:

tls: failed to verify certificate: x509: failed to load system roots and no roots provided; open /etc/ssl/certs/ca-certificates.crt: permission denied

After upgrading to 1.9.1 -> 1.9.3. Temporarily solved it by setting disable_filesystem_isolation = true, which probably is not a permanent fix or good idea?

On Fedora CoreOS machines I do not have this issue (yet).

Flatcar info:

Flatcar Container Linux by Kinvolk stable 4081.2.0 for VMware
core@app03 ~ $ uname -a
Linux app03 6.6.60-flatcar #1 SMP PREEMPT_DYNAMIC Tue Nov 12 16:20:46 -00 2024 x86_64 Intel(R) Xeon(R) Gold 6138 CPU @ 2.00GHz GenuineIntel GNU/Linux

Fedora CoreOS info:

Fedora CoreOS 41.20241027.3.0
core@app04:~$ uname -a
Linux app04 6.11.5-300.fc41.x86_64 #1 SMP PREEMPT_DYNAMIC Tue Oct 22 20:11:15 UTC 2024 x86_64 GNU/Linux

@tgross
Copy link
Member

tgross commented Nov 14, 2024

@ahjohannessen the sandbox controls weren't really impacted by this change other than chowning the artifact. You're not even getting to that step. Can you open a new issue please?

@ahjohannessen
Copy link

@tgross Opened here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

permissions errors with unveil on task directory
3 participants